fix: scaffolded-project path assumptions (#283, #284, #287, #288, #290) - #289
Open
antosubash wants to merge 9 commits into
Open
fix: scaffolded-project path assumptions (#283, #284, #287, #288, #290)#289antosubash wants to merge 9 commits into
antosubash wants to merge 9 commits into
Conversation
Four reports from a `sm new project` scaffold, all the same root cause: the framework assumed its own monorepo layout, where a module's directory and assembly are both `SimpleModule.<Module>`. `sm new module` scaffolds a bare `<Module>` for both. Page bundle 404 (#287) The client guessed `_content/SimpleModule.<Module>/`, but an RCL serves its assets under its AssemblyName, so every scaffolded module 404'd on first load. The renderer now declares the module name -> assembly name mapping in the page shell and the resolver reads it, so the first request goes to the path that actually serves the bundle. Probing remains as a fallback for shells that predate the map. The same prefix assumption also kept a bare-named module's stylesheet from ever being linked; module CSS discovery now consults the same mapping. types.ts written to a directory no project owns (#283, #284) extract-ts-types.mjs hardcoded `SimpleModule.<Module>` as the output project and created it unconditionally. It now resolves the project directory that actually exists and skips modules with no local source, so NuGet-installed modules no longer materialise phantom source trees in a consumer's repo and deleting a module no longer resurrects one. Removes the orphan modules/Identity/src/SimpleModule.Identity/, which held only a generated types.ts for a contracts-only module and belonged to no project. Tailwind never recompiled for new pages (#288) Styles/app.css declares module pages as @source globs, but the MSBuild target did not treat them as inputs, so a new page's utility classes were silently never generated. Module Pages/Views, the host ClientApp, and the staged module bundles are now inputs, with @(TailwindExtraSourceFiles) as an extension point. Verified: reproduced the stale-CSS failure on unmodified main and confirmed the fix compiles the class; exercised the extraction script against both layouts and a package-only module; confirmed the shell mapping and a single 200 for the page bundle in a browser. Full suite green (20 assemblies), npm run check clean.
…s error Follow-up to the review on #289. Tailwind up-to-date check was still incomplete. `Styles/app.css` declares five @source roots; only the module pages and ClientApp were tracked, so editing `packages/SimpleModule.UI` — the most-edited shared code in the repo — left the compiled CSS stale with exactly the #288 symptom. Adds the UI and client package roots (via `SimpleModuleUiDir`/`SimpleModuleClientDir`, which resolve to the npm packages in a scaffold and to `packages/` in the monorepo) and tracks `docs/design-system/` through the `@(TailwindExtraSourceFiles)` extension point. Moves the `Styles/_scan/**/*.js` glob into `CollectModuleAssets`. It was in an evaluation-time ItemGroup, but those files are staged during execution, so on the first build after installing a packaged module the glob expanded to nothing and `TailwindBuild` was judged up to date. Declaring the item in the target that stages them means the Inputs check, which runs afterwards, sees them. Anchors the module glob to `*/src/*/Pages/**` instead of `**/Pages/**` so evaluation no longer walks every module's bin/ and obj/ on every build, including design-time builds. `resolvePage` now reports the declared candidate's error rather than the last probe's. With the declaration authoritative, a real failure in the module's own bundle was being buried under a 404 from a fallback URL that was never going to resolve. Notes the scaffold's stripped `@source "./_scan/"` (#290) where the staging target could otherwise be misread as covering packaged modules everywhere.
Owner
Author
|
Addressed the review in 10bc18b. Four of six findings fixed; two declined with reasons. Fixed
Declined
One correction to my own verification. My first attempt to reproduce the untracked-UI-root gap reported a false pass — a leftover mtime from a previous probe, then incidental input churn that reruns Tailwind on many builds in this repo. A clean before/after via timestamps is not reliable here. The finding is settled by inspection instead (none of the committed globs cover Full suite green (20 assemblies, 0 failures), |
…s.ts - resolve-page: validate the parsed module-assembly map is a plain object, so a null/non-object payload memoizes as empty instead of throwing on every lookup - renderer: emit the map with JsonSerializer instead of hand-rolled JSON writing - targets: track ClientApp *.ts alongside *.tsx in the Tailwind input set - delete modules/Core/src/SimpleModule.Core/types.ts, a generator phantom with no csproj that nothing imports
The Tailwind output under wwwroot/ is already covered by .gitignore's "Tailwind CSS build output" rule, which allowlists only favicon.svg, index.html and error.html; app.css was tracked by accident. Widening the TailwindBuild input set made the target rerun on nearly every frontend edit, so the stale committed copy was rewritten constantly and every build produced a ~5000-line spurious diff. CI and Docker both compile the CSS themselves and the Host is IsPackable=false, so nothing depended on it. - resolve-page: narrow declared assembly names to non-empty strings so a module named after an Object.prototype member cannot build a junk URL - targets: track .js/.jsx under the UI and client packages, which are declared as directory @source roots and so scanned for every extension - test: key the assembly-map assertion on the page name's first segment, which is what the client looks up, instead of restating how it is built
…iour The comment said the guesses only apply when the shell predates the assembly map, but they are always appended after the declared name, so a declared bundle that fails still falls back to probing. That is deliberate — the declaredError reporting depends on it — so only the comment was wrong.
SimpleModule.Hosting.targets stages every module's built .pages.js into Styles/_scan/, which is the only way Tailwind sees utility classes coming from a packaged module or from components outside Pages/. The CLI stripped the matching @source line, so scaffolded apps rendered those classes unstyled. The path is relative to Styles/, so the same line is correct in both the monorepo and a scaffold. The line was originally left in place out of a concern that Tailwind would fail on a missing directory in a fresh scaffold; it does not — v4 skips absent @source roots without error, and CollectModuleAssets creates the folder before TailwindBuild runs regardless. Adds HostTemplatesAppCssTests covering the retained _scan source and the existing package/module path rewrites. SimpleModule.Cli.Tests sets IsTestProject=false, so run it with: dotnet run --project tests/SimpleModule.Cli.Tests
….css docs/design-system is tracked in the monorepo through @(TailwindExtraSourceFiles) and has no consumer equivalent, so copying the directive verbatim left a scaffold pointing at a directory that never exists — and would have silently started scanning unrelated files if a user ever created that path. Tailwind skips absent roots, so this was dead config rather than a build break. Also makes the sibling test's stated invariant true: no in-repo root survives into a scaffold.
Covers the #287 contract end to end: for each module route the shell must declare an assembly for that page's module, and the browser must fetch exactly that bundle, once, with a 200 — a second request would mean the client fell back to probing after a 404. Expectations are derived from the shell's own Inertia component name and assembly map rather than a hardcoded route table, so the test checks the contract instead of restating today's wiring. Also asserts Tailwind utilities used by module pages survive compilation, with an unused control class to prove the check discriminates (#288). Verified the spec fails when the map is emptied (5/5 route cases) and passes once restored.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #283, fixes #284, fixes #287, fixes #288, fixes #290.
Five reports filed against a
sm new projectscaffold, all one root cause: the framework assumed its own monorepo layout, where a module's directory and assembly are bothSimpleModule.<Module>.sm new modulescaffolds a bare<Module>for both.#287 — page bundle 404 on every page load
The client built the bundle URL as
_content/SimpleModule.<Module>/…, but an RCL serves its assets under itsAssemblyName, so every scaffolded module 404'd before the fallback probe found it.HtmlFileInertiaPageRenderernow declares the module-name → assembly-name mapping in the page shell, andresolvePagereads it, so the first request goes to the path that actually serves the bundle. Probing stays as a fallback for shells that predate the map, and for an unparseable one.The same prefix assumption also meant a bare-named module's stylesheet was never linked —
BuildModuleCssLinksonly matched_content/SimpleModule.*. Module CSS discovery now consults the same mapping.#283 / #284 —
types.tswritten into a directory no project ownsextract-ts-types.mjshardcodedSimpleModule.<Module>as the output project andmkdirSync'd it unconditionally. It now resolves the project directory that actually exists and skips modules with no local source, so:Pages/that import them (extract-ts-types.mjs writes types.ts into a SimpleModule.<Module> directory that sm new module never creates #284)generate:typesAlso removes two committed artifacts of the old behaviour —
modules/Identity/src/SimpleModule.Identity/andmodules/Core/src/SimpleModule.Core/— each holding only a generatedtypes.tsand belonging to no project.#288 — Tailwind never recompiled for new pages
Styles/app.cssdeclares its scan roots as@sourcedirectives, butTailwindBuilddid not treat them as inputs, so a new page's utility classes were silently never generated. All five declared roots are now inputs: modulePages//Views/, the hostClientApp, the@simplemodule/uiand@simplemodule/clientpackages, and the staged module bundles — with@(TailwindExtraSourceFiles)as an extension point for hosts that add@sourcedirectives of their own.template/SimpleModule.Host/wwwroot/css/app.cssis no longer tracked..gitignore's "Tailwind CSS build output" rule already excluded it (allowlisting onlyfavicon.svg,index.html,error.html); it had been force-added. With the input set widened, the target now reruns on nearly every frontend edit, so the stale committed copy was being rewritten constantly and produced a ~5,000-line spurious diff on every build. CI and Docker each compile the CSS themselves and the Host isIsPackable=false, so nothing depended on it.#290 — scaffolded hosts never scanned packaged modules' bundles
HostTemplates.AppCss()stripped the@source "./_scan/"line, so a scaffolded host staged every packaged module's built.pages.jsintoStyles/_scan/and then never scanned it — utility classes appearing only in a packaged module rendered unstyled. The path is relative toStyles/, so the same line is correct in both layouts.This was originally deferred out of a concern that Tailwind would fail on a missing directory in a fresh scaffold. It does not — v4 skips absent
@sourceroots without error (verified directly), andCollectModuleAssetscreates the folder beforeTailwindBuildruns regardless. The monorepo-onlydocs/design-systemroot is now stripped instead, since a scaffold has no such directory.Verification
main: addedbg-lime-700to a module page, rebuilt, class absent fromapp.css. With the fix,bg-fuchsia-800compiles in.src/<Name>(scaffold),src/SimpleModule.<Name>(framework), and a module with no source. Writes into the first two, skips and creates nothing for the third. Re-running against this repo produces zero changes to any existingtypes.ts.HostTemplatesAppCssTestsfails when the strip is restored and passes without it.Tests added
InertiaEndpoint_DeclaresAssemblyNameThatServesEachModuleBundle— asserts every module with pages is declared and every declared value is a real loaded assembly.tests/e2e/tests/smoke/module-bundles.spec.ts— end-to-end: for each module route the shell must declare an assembly for that page's module and the browser must fetch exactly that bundle, once, with a 200. Expectations are derived from the shell's own Inertia component name and assembly map rather than a hardcoded route table. Confirmed it fails (5/5 route cases) when the map is emptied. Also asserts Tailwind utilities survive compilation, with an unused control class proving the check discriminates.HostTemplatesAppCssTests— guards the retained_scansource and the scaffold path rewrites.SimpleModule.Cli.TestssetsIsTestProject=false, sodotnet testskipped it entirely and these would have guarded nothing; thebuildjob now runs that class explicitly.Local CI
Lint/format,
validate-pages,validate:i18n, framework-scope, typecheck 15/15,dotnet build0 warnings / 0 errors underTreatWarningsAsErrors, full suite green (20 assemblies, 1,309 tests, 0 failures), CLI template tests 4/4, e2e smoke suite 72/72.Out of scope
Review surfaced issues in code already on
mainthat this PR does not touch, left alone deliberately rather than bundled into a path-fix change:auth.show_test_accountshasDefaultValue = "true"with no environment gate andLoginEndpointfalls back to it, whileLogin.tsxhardcodesadmin@simplemodule.dev/Admin123!and the seeder now always applies that default (fix: shutdown-noise in background services + seed defaults so the app starts out of the box #279). A Production container with no seed config renders a working "Login as Admin" button. Fixing it reverses part of the deliberate out-of-box decision, so it needs its own change.ProgressFlushServicere-readsmoduleOptions.Valuein the shutdown drain instead of the capturedopts.IsLocalOrTest's doc comment still describes fail-closed guards that now only warn.